Skip to content

fix(ci): recognize namespaced test attributes in pr-triage's needs-tests check#202

Open
euxaristia wants to merge 6 commits into
Gitlawb:mainfrom
euxaristia:fix/pr-triage-async-test-detection
Open

fix(ci): recognize namespaced test attributes in pr-triage's needs-tests check#202
euxaristia wants to merge 6 commits into
Gitlawb:mainfrom
euxaristia:fix/pr-triage-async-test-detection

Conversation

@euxaristia

@euxaristia euxaristia commented Jul 14, 2026

Copy link
Copy Markdown

Summary

The needs-tests check in .github/workflows/pr-triage.yml uses:

/^\+.*#\[(test\]|cfg\(test\))/m

to detect whether a PR added an inline test. This only matches a literal #[test] or #[cfg(test) immediately after #[, so it misses namespaced test attributes like #[tokio::test] or #[async_std::test]. Any PR whose only new tests are async gets falsely labeled needs-tests, even when tests were added in the same commit.

Closes #201

Reproduction

PR #198 added two #[tokio::test]-only tests to crates/gl/src/doctor.rs in the same commit as the source fix, and the triage bot still commented needs-tests.

Fix

/^\+.*(#\[test\]|#\[cfg\(test\)|::test\])/m

Matches any attribute ending in ::test], covering namespaced test macros at any depth, while still matching the original #[test] / #[cfg(test)] forms.

How a reviewer can verify

I don't see any existing test harness for the inline github-script workflows in this repo, so I verified the regex directly against representative diff patches (a standalone Node script, not committed — just the verification, shown here):

const oldRe = /^\+.*#\[(test\]|cfg\(test\))/m;
const newRe = /^\+.*(#\[test\]|#\[cfg\(test\)|::test\])/m;

// old=true, new=true  — "+    #[test]\n+    fn foo() {}"
// old=true, new=true  — "+#[cfg(test)]\n+mod tests {"
// old=false, new=true — "+    #[tokio::test]\n+    async fn foo() {}"   <- the bug
// old=false, new=true — "+    #[async_std::test]\n+    async fn foo() {}"
// old=false, new=false — "+    let x = 1;\n+    println!(\"{x}\");"    <- correctly still rejected
// old=false, new=true — PR #198's actual patch excerpt

All six cases behave as expected under the new regex, including the real PR #198 diff that triggered this.

Kind of change

  • Bug fix
  • Feature
  • Security fix
  • Docs
  • Tests / CI
  • Refactor (no behavior change)
  • Breaking or protocol change

Summary by CodeRabbit

  • Bug Fixes
    • Improved pull request triage detection for newly added Rust inline test gates.
    • More accurately recognizes #[cfg(test)] and async test attribute forms such as #[tokio::test(...)] and #[async_std::test].
    • Reduced false positives by analyzing added lines with context-aware matching and ignoring occurrences inside comments and string/char literals.

…sts check

The needs-tests detection regex only matched a literal #[test] or #[cfg(test)
immediately after #[, so it missed namespaced test attributes like
#[tokio::test] or #[async_std::test]. Any PR whose only new tests were async
(the common pattern in this codebase, e.g. crates/gl/src/agent.rs) got falsely
labeled needs-tests even when tests were added in the same commit.

Reproduced against PR Gitlawb#198: it added two #[tokio::test]-only tests in the same
commit as the source change, and the triage bot still commented needs-tests.

Widens the regex to also match any attribute ending in ::test], verified
against representative patches including PR Gitlawb#198's actual diff.

Closes Gitlawb#201
@euxaristia
euxaristia requested a review from beardthelion as a code owner July 14, 2026 15:21
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c1c72d88-3d0c-42ac-85bf-8c83d4480021

📥 Commits

Reviewing files that changed from the base of the PR and between 0f41f25 and 2d8b7f7.

📒 Files selected for processing (1)
  • .github/workflows/pr-triage.yml
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/pr-triage.yml

📝 Walkthrough

Walkthrough

The PR triage workflow replaces patch-text matching with source-aware Rust test detection that filters comments and literals, maps added lines, fetches head files, and recognizes cfg(test) and namespaced test attributes.

Changes

Rust test detection

Layer / File(s) Summary
Expand inline test attribute matching
.github/workflows/pr-triage.yml
The workflow adds an anchored regex for cfg(test) and namespaced test attributes.
Scan added Rust lines in source context
.github/workflows/pr-triage.yml
The workflow maps diff-added line numbers, lexically strips comments and literals, fetches head-version Rust files, and matches only genuinely added source attributes.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PRDiff
  participant TriageWorkflow
  participant GitHubDataAPI
  PRDiff->>TriageWorkflow: provide Rust patch
  TriageWorkflow->>TriageWorkflow: identify added line numbers
  TriageWorkflow->>GitHubDataAPI: fetch head-version Rust file
  GitHubDataAPI-->>TriageWorkflow: return file content
  TriageWorkflow->>TriageWorkflow: strip comments and literals
  TriageWorkflow->>TriageWorkflow: detect added test attributes
Loading

Possibly related PRs

  • Gitlawb/node#58: Adds the initial PR triage workflow and its Rust test-signal detection.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly states the CI triage fix for recognizing namespaced test attributes in the needs-tests check.
Description check ✅ Passed The PR description includes summary, repro, fix, verification, and change kind; it is mostly complete despite some optional sections being sparse.
Linked Issues check ✅ Passed The change satisfies #201 by detecting #[test], #[cfg(test)], and namespaced ::test attributes while filtering unrelated Rust text.
Out of Scope Changes check ✅ Passed The extra workflow logic stays focused on more accurate inline-test detection and is still relevant to the linked CI issue.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the workflow-change Fork PR edits CI workflows — mandatory human review label Jul 14, 2026

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified by running both the old and new regex over the full addsInlineTest predicate (filename guard, f.patch guard, ^\+ anchor, .some(), and the alternation), 20+ inputs each way. The direction is right and the dominant #[tokio::test] form now resolves with zero regressions. Two things to fold in before merge.

Findings

  • [P2] Cover the parametrized #[tokio::test(flavor = ...)] form
    .github/workflows/pr-triage.yml:78
    The ::test] alternative needs ] immediately after test, but the args form is ::test(...), so test] never appears and it's missed. This is the PR's own target class and it's live in the tree at crates/gitlawb-node/src/git/smart_http.rs:753 and :818. A PR whose only new tests use the flavor form still gets falsely labeled needs-tests. The bare form (248 hits) is fixed, so this is a partial fix rather than a wrong one.

  • [P3] Anchor the match to an attribute so ::test] stops over-matching non-test code
    .github/workflows/pr-triage.yml:78
    ::test] is unanchored, so it also fires on ordinary added lines like let r = registry[Region::test]; and on the macro appearing in a comment or string literal. Effect is bounded (it only suppresses the advisory label, which isn't a merge gate), but it's a new false-positive class the old regex didn't have.

Both close with a single regex. I ran this against the same matrix: 0 regressions, 0 false-positives, and it catches the flavor form:

/^\+\s*#\[\s*(cfg\(\s*test|[\w-]+(::[\w-]+)*::test|test)\b/m

Anchoring to a leading #[ removes the over-match, and ::test\b (instead of ::test]) also clears #[tokio::test(...)].

Net: strict improvement as it stands, and the above makes it complete against the title. Non-gating label, so nothing here is severe, but the flavor form defeats the stated purpose on code that already exists in the repo.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found issues that need to be addressed before this is ready.

Findings

  • [P3] Keep the detector on an added diff line
    .github/workflows/pr-triage.yml:80
    \s* after the diff + also consumes newlines. As a result, a Rust PR can add only a blank line immediately before an existing context-line #[tokio::test]; the expression then treats that unchanged test as newly added and suppresses needs-tests. Use horizontal whitespace (for example, [ \t]*) at this position so the attribute itself must be on an added line.

  • [P3] Complete the cfg(test) predicate before accepting it as a test
    .github/workflows/pr-triage.yml:80
    The widened cfg\(\s*test\b branch also matches valid non-test predicates such as #[cfg(test = "triage_only")]. Adding only that attribute to an otherwise testless Rust PR makes addsInlineTest true and bypasses the advisory test signal; the former expression did not do so. Require the predicate to close (for example, cfg\(\s*test\s*\)) before classifying it as a test gate.

  • [P3] Handle valid whitespace in the namespaced attribute syntax
    .github/workflows/pr-triage.yml:80
    Rust accepts token whitespace in attributes, including #[tokio :: test] and #[cfg (test)], but the new matcher requires both :: and cfg( to be adjacent. A PR whose only added test uses either valid form is still labeled needs-tests, so the claimed namespaced/cfg recognition remains incomplete. Allow horizontal token whitespace around these separators while keeping the match confined to the added line.

  • [P3] Do not treat raw-string contents as executable test attributes
    .github/workflows/pr-triage.yml:80
    Anchoring at #[ prevents ordinary comment and string-expression lines from matching, but it does not distinguish a multiline raw string: an added fixture containing a line #[tokio::test] makes this expression true despite adding no runnable test. This is a new false positive for namespaced attributes and lets a testless Rust change suppress needs-tests; account for Rust string state (or otherwise avoid presenting the leading-attribute check as excluding strings).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/pr-triage.yml:
- Around line 83-87: Update the patch-scanning logic around the lexical state
object and f.patch iteration so state is not carried across incomplete hunks.
Reconstruct lexical context from the complete head-version file using added-line
coordinates, or reset and initialize state from each hunk’s surrounding content
before evaluating additions, ensuring #[test] text is classified correctly when
a patch begins inside a comment or multiline literal.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d71643b0-2e39-45be-b0ae-f0e11deb6339

📥 Commits

Reviewing files that changed from the base of the PR and between 3927e65 and 3cd2674.

📒 Files selected for processing (1)
  • .github/workflows/pr-triage.yml

Comment thread .github/workflows/pr-triage.yml Outdated
A diff hunk carries no lexical context from lines outside it, so scanning
patch text alone could misclassify an added line that sits inside a comment
or raw string opened earlier in the file (or outside the hunk's context
window). Fetch the head-version file content via the contents API and lex it
in file order, checking only the lines the diff actually added.
@euxaristia

Copy link
Copy Markdown
Author

Pushed a fixup addressing jatmn's four findings and this morning's CodeRabbit finding:

  • The regex now requires only horizontal whitespace before #[, so a blank added line before an unchanged context-line test no longer counts as adding a test.
  • cfg(test...) now has to close before it's accepted as a test gate, so #[cfg(test = "triage_only")] no longer counts.
  • Whitespace around :: and inside cfg( is now allowed, matching valid Rust like #[tokio :: test] and #[cfg (test)].
  • Added a small Rust lexer (line comments, block comments, char literals, normal and raw strings) so attribute-looking text inside string/comment content is no longer mistaken for a real test.

For the lexer's cross-hunk state gap CodeRabbit flagged: instead of scanning the diff patch text directly, the workflow now fetches the head-version file content via the contents API and lexes it in file order, then checks only the line numbers the diff actually added (computed from the hunk headers). This means lexical state (open raw string, block comment, etc.) is always accurate regardless of where a hunk's context window starts.

Verified against 12 cases covering all of the above, including the specific scenario CodeRabbit described (an added line landing inside a raw string opened outside the hunk's visible context).

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found issues that need to be addressed before this is ready.

Findings

  • [P2] Preserve nesting while skipping block comments
    .github/workflows/pr-triage.yml:89
    Rust block comments nest, but this lexer represents comment state as a boolean and clears it at the first */. A source-only PR can add #[tokio::test] after an inner comment closes but before the enclosing comment closes (/* outer, /* inner */, #[tokio::test], */); Rust still treats the attribute as comment text, while this code treats it as live and suppresses needs-tests. Track block-comment depth (and cover nested comments) before accepting an attribute.

  • [P2] Bound the per-file Contents API scan and do not silently classify failures as no test
    .github/workflows/pr-triage.yml:170
    Every changed Rust file with a patch now causes a serial repos.getContent call, while every failure is silently skipped. A large PR can therefore exhaust the workflow's ten-minute/API budget, and rate-limit or transient failures on the only file containing a new test make the triage job incorrectly add needs-tests (or prevent label reconciliation altogether). The previous implementation only examined the already-fetched file patches. Put a bounded strategy/fallback around this scan and surface a failure rather than treating an unavailable file as testless.

  • [P3] Require test to be the terminal attribute segment
    .github/workflows/pr-triage.yml:80
    The test\b alternatives stop at a word boundary rather than a valid attribute terminator, so non-test attributes such as #[foo::test::helper], #[test::helper], and #[foo::test = "x"] match. Adding one of those to a Rust-only PR suppresses the advisory test signal despite adding no test. Require optional horizontal space followed by ] or ( after the bare/namespaced test segment.

  • [P3] Recognize valid multiline test attributes
    .github/workflows/pr-triage.yml:80
    Rust permits whitespace, including newlines, between attribute tokens. A valid added test formatted as #\n[test] or #[tokio\n::test] is scanned one line at a time and neither line matches this horizontal-whitespace-only expression, so the PR still receives needs-tests. Carry enough attribute-token state across added lines (or parse the added attribute span) to cover the valid forms the workflow is intended to recognize.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found issues that need to be addressed before this is ready.

Findings

  • [P2] Do not abort triage when the bounded scan is exhausted
    .github/workflows/pr-triage.yml:199
    The cap is reached using the broad test/cfg substring prefilter rather than a real attribute match. A normal Rust PR that edits 16 files with (for example) #[cfg(feature = ...)] or even comments containing those words reaches the throw on the sixteenth file. That exits before the label reconciliation below, so every managed label (including stale labels that should be removed) and the guidance comment are left untouched. Keep the API work bounded, but use a non-fatal conservative fallback so an otherwise valid multi-file PR is still triaged.

  • [P2] Handle Contents API responses that do not carry Base64 text
    .github/workflows/pr-triage.yml:206
    getContent is decoded unconditionally, but GitHub returns an empty content with encoding: "none" for the 1–100 MB range unless the request uses the raw representation. An added #[tokio::test] in such a Rust file therefore gets lexed as an empty file and is incorrectly labeled needs-tests (and an absent content response would instead fail the job). Check the response encoding/size and fetch a supported raw/blob representation or take an explicit conservative fallback.

  • [P3] Require an added attribute token, not merely added whitespace inside one
    .github/workflows/pr-triage.yml:226
    The expression permits newlines between # and [, then marks a match as new when any character in its span is on an added line. Thus a source-only PR can insert just a blank line into an existing multiline attribute (#, added blank line, [test]) and make addsInlineTest true without adding a test. Rust accepts that formatting, so this suppresses needs-tests for the rest of an otherwise testless change. Tie the match to an added attribute token/span rather than any whitespace it crosses.

  • [P3] Preserve token boundaries while stripping literals
    .github/workflows/pr-triage.yml:136
    Removing a literal without a placeholder can join unrelated Rust tokens into a synthetic test attribute. For example, consume!(# "not an attribute" [test]); is valid input to a macro_rules! consume { ($($tt:tt)*) => {}; } matcher, but the stripper produces #[test]; the regex then treats the added invocation as a test. Preserve a separator for skipped comments/literals (or match lexed attribute tokens) so testless macro changes cannot bypass the advisory label.

  • [P3] Recognize absolute namespaced test attributes
    .github/workflows/pr-triage.yml:80
    Rust accepts an attribute path beginning with ::, but the namespaced branch requires an identifier before its first ::. Consequently #[::tokio::test] is not detected even though it is a valid async-test attribute, and a PR that adds only that form still receives needs-tests. Allow the optional leading path separator when matching a namespaced test attribute.

…n matching

Addresses jatmn's second round of review on pr-triage's needs-tests check:
hitting the per-PR fetch cap now logs and stops scanning instead of
throwing (which was skipping label reconciliation entirely), file content
comes from the Git blob API instead of the Contents API to avoid the
encoding: none gap for 1-100MB files, the added-line check now requires a
non-whitespace attribute character (not just added whitespace) on the
added line, stripped literals leave a placeholder so they can't glue
adjacent tokens into a false match, and the namespaced-test regex now
accepts a leading :: for absolute-path attributes.
@euxaristia

Copy link
Copy Markdown
Author

Pushed a fixup addressing this round of jatmn's findings:

  • The bounded Contents API scan no longer aborts the whole job when the fetch cap is hit; it now logs a warning and stops scanning further candidates, so label reconciliation and the guidance comment still run for the rest of the PR.
  • File content now comes from the Git blob API (using the blob sha already in the diff) instead of the Contents API, avoiding the encoding: "none" gap that API has for files between 1-100MB. A non-base64 response is logged and skipped rather than silently treated as empty.
  • The added-line check now requires an actual non-whitespace attribute character to be on an added line, not just any whitespace the match happens to span, so an added blank line inside an otherwise-unchanged multiline attribute no longer counts as adding a test.
  • Stripped comments/strings/char literals now leave a placeholder character behind, so a macro argument list containing a stray # and a string literal can't have its stripped literal glue the surrounding tokens into a false #[test] match.
  • The namespaced-test regex now accepts an optional leading ::, so #[::tokio::test] is recognized.

Verified with a 22-case regression harness run directly against the functions extracted from this file (all prior cases plus one per new finding), plus a syntax check of the extracted script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

workflow-change Fork PR edits CI workflows — mandatory human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PR triage's test-detection regex misses namespaced test attributes (#[tokio::test], etc.)

3 participants